EDM De-Algebraification #1, main branch (2026.04.08.)#1287
EDM De-Algebraification #1, main branch (2026.04.08.)#1287krasznaa wants to merge 10 commits intoacts-project:mainfrom
Conversation
krasznaa
force-pushed
the
EDMDeAlgebraification-main-20260401
branch
from
f0da9fb to
de91c95
Compare
stephenswat
left a comment
stephenswat
left a comment
There was a problem hiding this comment.
Removing the algebra from the EDM is largely uncontroversial, so I'm on board with this. The only real issue with this PR is that it tasks the EDM with the responsibility of type conversions where that should really be the callers' responsibility.
krasznaa
force-pushed
the
EDMDeAlgebraification-main-20260401
branch
from
de91c95 to
edcd9a6
Compare
stephenswat
left a comment
stephenswat
left a comment
There was a problem hiding this comment.
Looks much nicer, thanks.
| template <detray::concepts::algebra algebra_t, typename measurement_backend_t> | ||
| TRACCC_HOST_DEVICE void get_measurement_local( | ||
| const edm::measurement<measurement_backend_t>& meas, | ||
| detray::dpoint2D<algebra_t>& pos); |
There was a problem hiding this comment.
Is there a good reason why this cannot return the dpoint2D by value?
There was a problem hiding this comment.
I'm trying to balance between writing code in the style of existing code, and writing it in a way that I would like. 🤔 But indeed, while the existing API makes sense for variable sized matrices, it's indeed not great for fixed sized vectors.
Made both of them use a concrete, FP32 interface. One that doesn't depend on how traccc was built exactly. Updated all clients in the core library to successfully use that fixed interface. Even when traccc is built in FP64 mode.
According to PR recommendations.
krasznaa
force-pushed
the
EDMDeAlgebraification-main-20260401
branch
from
edcd9a6 to
68a6a71
Compare
|
stephenswat
left a comment
stephenswat
left a comment
There was a problem hiding this comment.
Looks good, let's go with this. 👍
2 participants
With this let me start yet another big rewrite / redesign. 🤔
It's been bugging me for a long time that the data model we use depends on the algebra that we use. Which in my mind is not a workable API. The data itself needs to be fixed somehow.
So I started by making
traccc::edm::silicon_cellandtraccc::edm::measurementnot be dependent on the build setup of the project. As these seemed technically the easiest to do, and sinceedm::measurementis used pretty much everywhere, I thought that this would be a good stress-test for the rewrite.Making
edm::silicon_cellalways usefloatvalues for its "activation" and "time" properties was a trivial change, that didn't actually necessitate any code changes. So that was surprisingly easy.But of course
edm::measurementwas a bit more tricky. What we do currently is that we use thedetray::point2Dtype for storing the local coordinates and the variances of those local coordinates. But I believe it's a lot better idea to just store plainstd::array<float, 2>objects. Independent of what algebra we want to use for our calculations.To help with this, I added a couple of additional functions to
edm::measurement. Which would help with both retrieving and setting thelocal_positionandlocal_variancevariables using Detray objects directly. Just so that the actual algorithmic code would look a bit more "natural". Like:(Okay, maybe "natural" is a bit over-selling it. 🤔)
I see this all as one step towards hiding the whole algebra business behind the scenes. As the API of the GPU tools should really not reveal what algebra is being used underneath. In case we end up using array and Eigen algebras interchangeably in the end after all. 🤔
I assume there will be plenty of discussion around this. Especially around what we'll do with
edm::track_stateafterwards. (As there I do have some worries about what we actually want to do.) But let's see how this goes...